Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(rewrites): change TableColumn -> Field in rewrites #8448

Merged
merged 1 commit into from
Feb 27, 2024
Merged

fix(rewrites): change TableColumn -> Field in rewrites #8448

merged 1 commit into from
Feb 27, 2024

Conversation

gforsyth
Copy link
Member

Description of changes

I don't know if just aren't using these rewrites or aren't testing them.

Anyway, quick PR to remove the now renamed ops.TableColumn -- if it makes more sense to rip out the rewrites, I can do that instead.

Issues closed

@github-actions GitHub Actions
Copy link
Contributor

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@gforsyth gforsyth changed the title fix(rewrites): TableColumn -> Field in rewrites fix(rewrites): change TableColumn -> Field in rewrites Feb 26, 2024
@kszucs
Copy link
Member

kszucs commented Feb 26, 2024

Apparently we don't use rewrite_dropna and rewrite_fillna anymore. We should check whether there are backends not supporting FillNa and DropNa and if there are, we can keep these rewrites.

@jcrist
Copy link
Member

jcrist commented Feb 26, 2024

Yeah, these got dropped in the refactor. I think we might want to bring them back though instead of implementing in the compiler (see

ibis/ibis/backends/sql/compiler.py

Lines 1319 to 1336 in e4edc78

def visit_FillNa(self, op, *, parent, replacements):
if isinstance(replacements, Mapping):
mapping = replacements
else:
mapping = {
name: replacements
for name, dtype in op.schema.items()
if dtype.nullable
}
exprs = {
col: (
self.f.coalesce(sg.column(col, quoted=self.quoted), sge.convert(alt))
if (alt := mapping.get(col)) is not None
else sg.column(col, quoted=self.quoted)
)
for col in op.schema.keys()
}
return sg.select(*self._cleanup_names(exprs)).from_(parent)
) since doing this via a rewrite may allow fusing selects to generate nicer SQL. Doing this during compilation doesn't allow for that.

That said, happy to merge the fix as is (or delete the rewrites fully until/if we want them back).

@gforsyth
Copy link
Member Author

I can look at restoring the expr-level rewrites

@kszucs
Copy link
Member

kszucs commented Feb 26, 2024

I think we might want to bring them back though instead of implementing in the compiler

SGTM, could you create a ticket to track this? I think we can merge it as is.

@gforsyth
Copy link
Member Author

SGTM, could you create a ticket to track this? I think we can merge it as is.

will do

@gforsyth gforsyth mentioned this pull request Feb 26, 2024
Closed
1 task
@kszucs kszucs changed the title fix(rewrites): change TableColumn -> Field in rewrites fix(rewrites): change TableColumn -> Field in rewrites Feb 27, 2024
@kszucs kszucs merged commit 3730eb6 into ibis-project:main Feb 27, 2024
74 of 76 checks passed
@gforsyth gforsyth deleted the no_more_tablecolumn branch February 27, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kszucs kszucs kszucs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gforsyth @kszucs @jcrist